Remove MessageInfo argument from migrate entry point #709

juggernaut09 · 2021-01-12T13:06:38Z

Closes #690

Changed the function signature of fn migrate as fn query. (Removed the MessageInfo). in both hackatom and burner contracts.
Changed the struct member verifier to payout as referenced by @webmaster128. in hackatom contract.
Updated the json schema of MigrateMsg (migrate_msg.json) in hackatom contract.
Update the entry points, both the macro(packages/std/src/entry_points.rs) and as well as the wrapper(packages/std/src/exports.rs).

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

Targeted PR against correct branch (see CONTRIBUTING.md)
Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
Code follows the module structure standards.
Wrote unit and integration tests
Updated relevant documentation (docs/) or specification (x//spec/)
Added relevant rustdoc comments.
Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
Re-reviewed Files changed in the Github PR explorer
Review Codecov Report in the comment section below once CI passes

…he migrate function signature as query function. Updated the json schema of MigrateMsg.

…ed the fn migrate signature in burner contract.

webmaster128

Looks good to me. Could you rebase this work to the latest master?

contracts/hackatom/src/contract.rs

ethanfrey

Thanks for the contribution

packages/std/src/exports.rs

…sue#690

juggernaut09 · 2021-01-15T07:47:27Z

Looks good to me. Could you rebase this work to the latest master?

@webmaster128 can you please review it again. I have rebased it to latest master branch. Solved merge conflicts also.

ethanfrey · 2021-01-15T09:13:32Z

Looks good to me. I would just add a CHANGELOG entry "removed info argument from migrate".
But Simon need to give the ✔️ to merge

webmaster128

Looks good, except for this diff in Building.md.

Would you also do #715? Ideally we do #715 first (independent PR) because it will ensure the change here works as expected.

Building.md

webmaster128

Looks good, thank you.

Now, why is #715 important? This PR (#709) changes the interface between VM and contract. The testing contract in packages/vm/testdata/hackatom_0.14.wasm is still the old one, i.e. incompatible. So updating packages/vm/testdata/hackatom_0.14.wasm should be required as part of this. However, this interface is not tested so this incompatibility is not detected by th CI.

juggernaut09 · 2021-01-18T07:48:16Z

Looks good, thank you.

Now, why is #715 important? This PR (#709) changes the interface between VM and contract. The testing contract in packages/vm/testdata/hackatom_0.14.wasm is still the old one, i.e. incompatible. So updating packages/vm/testdata/hackatom_0.14.wasm should be required as part of this. However, this interface is not tested so this incompatibility is not detected by th CI.

Ohk @webmaster128 I will work on the issue #715 and raise a PR.

webmaster128 · 2021-01-19T15:07:25Z

Could you update this to the latest master (rebase or merge master into this). This probably leads to one failing test. We then re-compile the testing contract to make it work again.

…e#690 pulling rebase code from remote main branch into local issue#690 after merging issue#715 code.

juggernaut09 · 2021-01-20T11:49:57Z

Could you update this to the latest master (rebase or merge master into this). This probably leads to one failing test. We then re-compile the testing contract to make it work again.

@webmaster128 I have tried to remove info: MessageInfo from fn call_migrate_works in packages/vm/src/calls.rsIt is raising a signature error because of passing only env and msg to call_raw. as below
call_raw(instance, "migrate", &[env, msg], MAX_LENGTH_MIGRATE)

The error is Parameters of type [I32, I32] did not match signature [I32, I32, I32] -> [I32]"
It is causing by the following code snippet.
https://github.com/wasmerio/wasmer/blob/1.0.1/lib/api/src/externals/function.rs#L500-L506

webmaster128 · 2021-01-20T14:47:28Z

@juggernaut09 right, this is because the test contract packages/vm/testdata/hackatom_0.14.wasm needs to be recompiled when the interface between VM and contract changes. You can do this as followes (see https://github.com/CosmWasm/cosmwasm/blob/master/packages/vm/README.md#setup):

docker run --rm -v "$(pwd)":/code \
  --mount type=volume,source="devcontract_cache_hackatom",target=/code/contracts/hackatom/target \
  --mount type=volume,source=registry_cache,target=/usr/local/cargo/registry \
  cosmwasm/rust-optimizer:0.10.7 ./contracts/hackatom \
  && cp artifacts/hackatom.wasm packages/vm/testdata/hackatom_0.14.wasm

After that, it should work. You will have to update some gas values in the tests after recompiling.

…igrate_works.

webmaster128

Looks good to me. Just the debug print

packages/vm/src/calls.rs

webmaster128 · 2021-01-20T17:05:57Z

Thanks a lot for your contribution, @juggernaut09

juggernaut09 added 3 commits January 12, 2021 17:32

changed the attribute verifier to payout in MigrateMsg. and changed t…

c80c004

…he migrate function signature as query function. Updated the json schema of MigrateMsg.

updated the ffi entry points both macro as well as the wrapper. Chang…

ff3a524

…ed the fn migrate signature in burner contract.

Solved all fmt_issues, new_verifier is replaced by new_payout.

e45549e

juggernaut09 marked this pull request as ready for review January 12, 2021 13:51

webmaster128 reviewed Jan 12, 2021

View reviewed changes

contracts/hackatom/src/contract.rs Outdated Show resolved Hide resolved

webmaster128 added the Breaking (interface) Breaks VM-contract interface, likely requires `cosmwasm_version` bump label Jan 12, 2021

ethanfrey reviewed Jan 12, 2021

View reviewed changes

packages/std/src/exports.rs Show resolved Hide resolved

juggernaut09 added 5 commits January 15, 2021 11:27

Changed payout to verifier in hackatom contract.

1de3577

Replaced all payout with verifier. in tests/integration also.

83e7bdf

Merge branch 'master' of https://github.com/CosmWasm/cosmwasm into is…

327cc8c

…sue#690

resolved all circle_ci tests for contract_queue

d0b0412

resolved fmt_changes

cc57c28

juggernaut09 requested a review from webmaster128 January 15, 2021 07:43

webmaster128 reviewed Jan 17, 2021

View reviewed changes

Building.md Outdated Show resolved Hide resolved

Removed unnecessary comments from Building.md

84b5902

juggernaut09 requested a review from webmaster128 January 18, 2021 07:16

webmaster128 reviewed Jan 18, 2021

View reviewed changes

webmaster128 changed the title ~~Issue#690~~ Remove MessageInfo argument from migrate entry point Jan 18, 2021

Base automatically changed from master to main January 19, 2021 22:43

juggernaut09 added 2 commits January 20, 2021 14:58

Merge branch 'main' of https://github.com/CosmWasm/cosmwasm into issu…

dc8c8da

…e#690 pulling rebase code from remote main branch into local issue#690 after merging issue#715 code.

removed info from "call_migrate_works".

fd08b54

Removed MessageInfo argument from migrate entry point, Updated call_m…

fd7600a

…igrate_works.

webmaster128 reviewed Jan 20, 2021

View reviewed changes

packages/vm/src/calls.rs Outdated Show resolved Hide resolved

Removed debug statement.

ab919c3

webmaster128 mentioned this pull request Jan 20, 2021

Don't pass MessageInfo to migrate #690

Closed

2 tasks

webmaster128 approved these changes Jan 20, 2021

View reviewed changes

webmaster128 added the automerge See mergify.io label Jan 20, 2021

mergify bot merged commit 7e68844 into CosmWasm:main Jan 20, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove MessageInfo argument from migrate entry point #709

Remove MessageInfo argument from migrate entry point #709

juggernaut09 commented Jan 12, 2021 •

edited

Loading

webmaster128 left a comment

ethanfrey left a comment

juggernaut09 commented Jan 15, 2021

ethanfrey commented Jan 15, 2021

webmaster128 left a comment

webmaster128 left a comment

juggernaut09 commented Jan 18, 2021

webmaster128 commented Jan 19, 2021

juggernaut09 commented Jan 20, 2021 •

edited

Loading

webmaster128 commented Jan 20, 2021 •

edited

Loading

webmaster128 left a comment

webmaster128 commented Jan 20, 2021

Remove MessageInfo argument from migrate entry point #709

Remove MessageInfo argument from migrate entry point #709

Conversation

juggernaut09 commented Jan 12, 2021 • edited Loading

webmaster128 left a comment

Choose a reason for hiding this comment

ethanfrey left a comment

Choose a reason for hiding this comment

juggernaut09 commented Jan 15, 2021

ethanfrey commented Jan 15, 2021

webmaster128 left a comment

Choose a reason for hiding this comment

webmaster128 left a comment

Choose a reason for hiding this comment

juggernaut09 commented Jan 18, 2021

webmaster128 commented Jan 19, 2021

juggernaut09 commented Jan 20, 2021 • edited Loading

webmaster128 commented Jan 20, 2021 • edited Loading

webmaster128 left a comment

Choose a reason for hiding this comment

webmaster128 commented Jan 20, 2021

juggernaut09 commented Jan 12, 2021 •

edited

Loading

juggernaut09 commented Jan 20, 2021 •

edited

Loading

webmaster128 commented Jan 20, 2021 •

edited

Loading